Fixed #8546 and also fixed #8552#8590
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
step,_simple_print_message_rolenow always logsself.run_context.messages, so the[BefCompact]and[AftCompact]logs will be identical; consider passing/using the provider-processed_provider_messagesfor the post-compaction log so the debug output actually reflects the transformation. - The tool call filtering in
stepbuildsvalid_indicesfromzip(llm_resp.tools_call_name, llm_resp.tools_call_ids)but then indexes intollm_resp.tools_call_argsusing those indices; iftools_call_argsis shorter thantools_call_name/tools_call_ids, this will raiseIndexError, so it would be safer to derive indices from zipping all three lists or clamp to the minimum length. - The new
_should_fix_modalities_for_provideronly treats a non-emptylistas a configured modalities value, whereas previously any truthy value (including non-lists) triggered sanitization; if there are providers with non-listmodalitiesconfigs, this silently changes behavior, so consider either validating/normalizingmodalitiesto a list or preserving the old truthiness semantics.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `step`, `_simple_print_message_role` now always logs `self.run_context.messages`, so the `[BefCompact]` and `[AftCompact]` logs will be identical; consider passing/using the provider-processed `_provider_messages` for the post-compaction log so the debug output actually reflects the transformation.
- The tool call filtering in `step` builds `valid_indices` from `zip(llm_resp.tools_call_name, llm_resp.tools_call_ids)` but then indexes into `llm_resp.tools_call_args` using those indices; if `tools_call_args` is shorter than `tools_call_name`/`tools_call_ids`, this will raise `IndexError`, so it would be safer to derive indices from zipping all three lists or clamp to the minimum length.
- The new `_should_fix_modalities_for_provider` only treats a non-empty `list` as a configured modalities value, whereas previously any truthy value (including non-lists) triggered sanitization; if there are providers with non-list `modalities` configs, this silently changes behavior, so consider either validating/normalizing `modalities` to a list or preserving the old truthiness semantics.
## Individual Comments
### Comment 1
<location path="astrbot/core/agent/runners/tool_loop_agent_runner.py" line_range="596-598" />
<code_context>
log_context_sanitize_stats(stats)
return sanitized_contexts
+ def _should_fix_modalities_for_provider(self) -> bool:
+ modalities = self.provider.provider_config.get("modalities", None)
+ return (
+ isinstance(modalities, list) and modalities
+ ) # Empty list is treated as unconfigured
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The new modalities check only supports non-empty `list` values and will skip sanitization for other sequence types.
This narrows the definition of “configured” from any truthy value to only a non-empty `list`. If a provider config uses another sequence type (e.g. a tuple), sanitization will now silently stop applying. If that’s intended, consider normalizing `modalities` to a list earlier; otherwise, widen the check to accept other iterable sequence types (or coerce to `list`) so the previous behavior is preserved while still treating `None`/`[]` as unconfigured.
Suggested implementation:
```python
def _should_fix_modalities_for_provider(self) -> bool:
modalities = self.provider.provider_config.get("modalities", None)
# Treat None and empty sequences (e.g. [], (), etc.) as "unconfigured".
# For non-sequence types, preserve the previous "truthy means configured" behavior.
if modalities is None:
return False
try:
from collections.abc import Sequence # local import to avoid module-level changes if not already present
except ImportError: # very defensive; Sequence should be available in supported Python versions
return bool(modalities)
if isinstance(modalities, Sequence) and not isinstance(modalities, (str, bytes)):
return len(modalities) > 0
return bool(modalities)
```
If `Sequence` is already imported at the module level (e.g. `from collections.abc import Sequence`), you should remove the local import inside `_should_fix_modalities_for_provider` and rely on the existing import instead to keep imports centralized and avoid redundant imports.
</issue_to_address>
### Comment 2
<location path="astrbot/core/agent/runners/tool_loop_agent_runner.py" line_range="905-914" />
<code_context>
parts = None
+
+ # 过滤掉无效的 tool calls,确保 assistant 消息不包含无 id/name 的条目
+ if llm_resp.tools_call_name and llm_resp.tools_call_ids:
+ valid_indices = [
+ i for i, (name, tid) in enumerate(
+ zip(llm_resp.tools_call_name, llm_resp.tools_call_ids)
+ )
+ if name and tid
+ ]
+ if len(valid_indices) < len(llm_resp.tools_call_name):
+ llm_resp.tools_call_name = [llm_resp.tools_call_name[i] for i in valid_indices]
+ llm_resp.tools_call_args = [llm_resp.tools_call_args[i] for i in valid_indices]
+ llm_resp.tools_call_ids = [llm_resp.tools_call_ids[i] for i in valid_indices]
+
+ # 如果过滤后没有有效的 tool calls,跳过构建 tool_calls_result
</code_context>
<issue_to_address>
**issue (bug_risk):** Filtering tool calls by index assumes `tools_call_args` is at least as long as `tools_call_name`/`tools_call_ids`, which may raise `IndexError` on malformed provider output.
`valid_indices` comes from `zip(tools_call_name, tools_call_ids)`, but is then applied to `tools_call_args` without verifying that `tools_call_args` is at least as long. If a provider returns fewer args than names/ids, indexing `tools_call_args[i]` will raise `IndexError`. To avoid this, build the filtered lists in a single pass over `zip(tools_call_name, tools_call_args, tools_call_ids)` and append only fully valid triples, or use `zip_longest` with explicit handling of missing values.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request refactors the tool loop agent runner to prevent mutating canonical messages during compaction, updates the LLM compressor parameters, and adds filtering to skip invalid tool calls with missing names or IDs. The review feedback highlights several critical issues: a parameter renaming mismatch that will cause an AttributeError in ContextManager, potential IndexError exceptions when accessing tool call names and arguments without bounds checks, and a type annotation mismatch where a method returns a list instead of a boolean.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
首先,提交pr符合社区规范按照模板,不要起这种不知所云的标题。其次请原子化提交而不是把所有功能的修复和改动放到一个pr里。还有,配置证书和ssl这种事放在bot本体里会使得功能冗余,不适合作为一个feature放进来,这种事应该让nginx或者caddy来干。还有就是有很多无意义的改动。我将关闭这个pr,如果你愿意重新提交,请遵循社区规范使用模板,谢谢。 |

Fixed #8546 and also fixed #8552
Actually,
Fixed Pydantic data validation errors
Added validation for function.name being None and empty tool_call_id issues
In AstrBot/astrbot/core/computer/booters/local.py, a _BLOCKED_COMMAND_PATTERNS list was defined; users can now customize this list
Changed the health mode default to false — major mistake! Accidentally committed cmd_config.json! I won't change this config for now. If other collaborators (not contributors) request it, it can be modified.
After visiting http://X.X.X.X:6185/#/platforms and clicking "Create Bot" then selecting a messaging platform category, Telegram is placed at the very top — far ahead of others.
Regarding step limits and boring prompts (#8549), you can see the maximum step count is currently 30 — this is unreasonable and should be changed to 114514
Automatically apply for free Let's Encrypt SSL certificates, which means you can access it via https://X.X.X.X
修复了 #8546 也修复了 #8552